Skip to content

Conversation

commit111
Copy link
Collaborator

@commit111 commit111 commented Dec 17, 2024

Adding try-catch to improve error messages given by check-sample workflow.

@raphaeltm
Copy link
Collaborator

I think #275 (comment) and #281 and this PR are all related.

I couldn't get #280 to pass checks when I was building it and I gave up because it wasn't clear to me what was going on which caused all those samples which used to be fine to suddenly be marked as failing.

The last time that happened there was a bug in the cli so the compose file check failed.

I think we have some new checks that actually run a deployment? Is that correct @jordanstephens? If so can we document how to prepare everything in a sample to pass the checks? (and/or add those thing to be automatically added to the checklist)

Copy link
Member

@jordanstephens jordanstephens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@commit111 @raphaeltm It looks like the underlying error is related to these lines in scripts/check-sample-files.sh:

if ! defang compose config > /dev/null 2>/dev/null; then
echo " - [ ] ${dir}compose.yaml contains errors"
fi

Let's update that script to give us more information about the errors it found, and also to note where the errors came from. Maybe something like this:

-      if ! defang compose config > /dev/null 2>/dev/null; then
-        echo " - [ ] ${dir}compose.yaml contains errors"
+      output=$(defang compose config 2>&1)
+      if [[ $? -ne 0 ]]; then
+        printf " - [ ] %scompose.yaml is not valid according to \`defang compose config\`:\n  \`\`\`%s\`\`\`\n\n" "${dir}" "$output"

@jordanstephens
Copy link
Member

@raphaeltm we made a few changes in this PR to address issues:

  1. we added the error output from defang compose config when running scripts/check-sample-files.sh
  2. we renamed the check_samples job in .github/workflows/publish-sample-template.yml to avoid conflict with check_samples in .github/workflows/check_sample.yml
  3. we removed check_samples from the required status checks because it only runs when a pr includes changes which match ./samples/**. In the case of this PR, no changes were made that match that glob, but we couldn't merge because that job had not run.

Copy link
Member

@jordanstephens jordanstephens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should move the nodejs code in .github/workflows/check-sample.yml into a new script file. Maybe something like scripts/check-sample.js, but it's not strictly necessary.

@raphaeltm
Copy link
Collaborator

I think we should move the nodejs code in .github/workflows/check-sample.yml into a new script file. Maybe something like scripts/check-sample.js, but it's not strictly necessary.

Yup. That makes a lot of sense.

Okay, I guess I might have been mixing up a few things. I'll poke around all the samples PRs and try to get a better sense of things.

@raphaeltm raphaeltm merged commit ac63a2c into main Dec 18, 2024
3 checks passed
@commit111 commit111 deleted the linda-check-samples-action branch April 3, 2025 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants